Skip to content

fix: wheel for windows #111

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jan 4, 2025
Merged

fix: wheel for windows #111

merged 4 commits into from
Jan 4, 2025

Conversation

Tieqiong
Copy link
Contributor

@Tieqiong Tieqiong commented Jan 2, 2025

@sbillinge This should fix diffpy/diffpy.pdfgui#238 and diffpy/diffpy.pdfgui#240 after another pypi release. Before merging this the workflow file still need to be changed. The one attached here is just for showing it works.

@bobleesj Can you help merging this workflow file into the release-script central workflow? Thanks!

These are the local test of the wheel produced here

Windows with python3.12:
image

Linux with python3.12:
image

Mac Arm with python3.12:
image

Tieqiong and others added 3 commits January 1, 2025 23:02
Copy link

github-actions bot commented Jan 2, 2025

Warning! No news item is found for this PR. If this is a user-facing change/feature/fix,
please add a news item by copying the format from news/TEMPLATE.rst.

@bobleesj
Copy link
Contributor

bobleesj commented Jan 2, 2025

Awesome Tieqiong. Yes, once merged, I will do copy and paste.

@Tieqiong
Copy link
Contributor Author

Tieqiong commented Jan 2, 2025

@bobleesj Maybe it's better to copy the workflow before this get merged, because I don't want the dummy workflow to be merged. I will delete it after Simon review it and before merge.

@bobleesj
Copy link
Contributor

bobleesj commented Jan 2, 2025

👍 understood.

Once @sbillinge gives it a quick review, we can try rc7 release of pdffit then? @Tieqiong

Copy link
Contributor

@sbillinge sbillinge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Tieqiong this is very exciting! I left a couple of housework kind of comments, but when they are addressed, this look good to go.

name: Wheel builder

on:
pull_request:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shall we do just pull_request and push into main? I think that is our standard on our other workflows

Copy link
Contributor Author

@Tieqiong Tieqiong Jan 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sbillinge I'll delete this workflow file before merging. This is just for showing the changes work here, and it would be easier for @bobleesj to copy to the central release-script. So details shouldn't matter too much.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Tieqiong Billingegroup/release-scripts#123

I copied and saved in the issue. pls confirm if that's all good.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bobleesj please see comment there

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a general rule I like taking care of details when we think of them, otherwise technical debt tends to just start piling up. But if this is going to be completely gone through again later and everything fixed, i htink it is ok to let things slide here.

setup.py Outdated
class CustomBuildExt(build_ext):
def run(self):
super().run()
gsl_path = os.environ.get("GSL_PATH") or os.path.join(os.environ.get("CONDA_PREFIX", ""), "Library")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer if we could use pathlib.Path() as much as possible for these operations, for easier future maintainability.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sbillinge I changed most of the os.path to pathlib. Please check, thanks

Verified

This commit was signed with the committer’s verified signature.
Copy link

codecov bot commented Jan 4, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.96%. Comparing base (154c87a) to head (7e12beb).
Report is 8 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #111   +/-   ##
=======================================
  Coverage   98.96%   98.96%           
=======================================
  Files           6        6           
  Lines        1261     1261           
=======================================
  Hits         1248     1248           
  Misses         13       13           

@sbillinge sbillinge merged commit 46c5082 into diffpy:main Jan 4, 2025
12 of 13 checks passed
@Tieqiong Tieqiong deleted the winwheel branch January 12, 2025 02:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Windows Python 3.11, 3.12 matrix CI fail due to ImportError: DLL load failed while importing pdffit2
3 participants